datafeed: check remote_cluster_client before cluster aliases in start #129601
Conversation
TransportStartDatafeedAction previously tried to validate remote index cluster names in datafeed jobs, before checking if the local cluster had remote_cluster_client role. Because this role enables retrieval of the remote cluster names, the validation step would always fail with a no-such-cluster exception. This was confusing. This change moves the remote_cluster_client check ahead of cluster name validation, and adds a test. Closes ES-11841
schase-es
added
>non-issue
:Distributed Coordination/Cluster Coordination
|
Including David Kyle as the last person who edited these, as an FYI... this area seems to have limited distributed coordination involvement in the past. |
There was a problem hiding this comment.
Some minor nits, but this LGTM. Please wait until we've got approval from someone in ML before merging though, because I'm not 100% familiar with this code.
(specific question to someone more familiar with the code would be - is this the best place to do the check? Node roles are immutable, so we could do the check when the job is created, but the master could change to one that does have the role between creation and starting, and then everything would be fine? so perhaps start is the right place?)
...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java
Outdated
Show resolved
Hide resolved
...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java
Outdated
Show resolved
Hide resolved
...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java
Outdated
Show resolved
Hide resolved
.../plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java
Show resolved
Hide resolved
...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java
Outdated
Show resolved
Hide resolved
...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java
Outdated
Show resolved
Hide resolved
...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java
Outdated
Show resolved
Hide resolved
- changed doc url to link github issue - added return after continuation send -- oops! - use junit methods for testing message/exception matching - moved the test to the xpack ml/qa folder - added a gradle build file that appears to work - removed excess override settings from RestTest
…elastic#129601) TransportStartDatafeedAction previously tried to validate remote index cluster names in datafeed jobs, before checking if the local cluster had remote_cluster_client role. Because this role enables retrieval of the remote cluster names, the validation step would always fail with a no-such-cluster exception. This was confusing. This change moves the remote_cluster_client check ahead of cluster name validation, and adds a test. Closes ES-11841 Closes elastic#121149
💚 Backport successful
|
|
Great -- everything is merged. Thanks Nick as usualy, and David especially for reviewing something from another team -- and helping me get started with a gradle! |
…#129601) (#129802) TransportStartDatafeedAction previously tried to validate remote index cluster names in datafeed jobs, before checking if the local cluster had remote_cluster_client role. Because this role enables retrieval of the remote cluster names, the validation step would always fail with a no-such-cluster exception. This was confusing. This change moves the remote_cluster_client check ahead of cluster name validation, and adds a test. Closes ES-11841 Closes #121149
…elastic#129601) TransportStartDatafeedAction previously tried to validate remote index cluster names in datafeed jobs, before checking if the local cluster had remote_cluster_client role. Because this role enables retrieval of the remote cluster names, the validation step would always fail with a no-such-cluster exception. This was confusing. This change moves the remote_cluster_client check ahead of cluster name validation, and adds a test. Closes ES-11841 Closes elastic#121149
…elastic#129601) TransportStartDatafeedAction previously tried to validate remote index cluster names in datafeed jobs, before checking if the local cluster had remote_cluster_client role. Because this role enables retrieval of the remote cluster names, the validation step would always fail with a no-such-cluster exception. This was confusing. This change moves the remote_cluster_client check ahead of cluster name validation, and adds a test. Closes ES-11841 Closes elastic#121149
|
FYI @kderusso and @valeriy42: The error message is now a 400-response with an explanatory response: The old error message I reproduced in testing (and which is now gone), was a 404 response: I did not write it to issue an IllegalArgumentException as mentioned earlier. While there are several code paths that do this, this API already had a check for this and an error message, but it was in the wrong order relative to cluster name validation. |
…elastic#129601) TransportStartDatafeedAction previously tried to validate remote index cluster names in datafeed jobs, before checking if the local cluster had remote_cluster_client role. Because this role enables retrieval of the remote cluster names, the validation step would always fail with a no-such-cluster exception. This was confusing. This change moves the remote_cluster_client check ahead of cluster name validation, and adds a test. Closes ES-11841 Closes elastic#121149
4 participants
TransportStartDatafeedAction previously tried to validate remote index cluster names in datafeed jobs, before checking if the local cluster had remote_cluster_client role. Because this role enables retrieval of the remote cluster names, the validation step would always fail with a no-such-cluster exception. This was confusing. This change moves the remote_cluster_client check ahead of cluster name validation, and adds a test.
Closes ES-11841
Closes: #121149